Skip to content

Conversation

@Vinish100
Copy link
Contributor

Reason for change: Integrate mp4Demux formally into AAMP
Test Procedure: Test with useMp4Demux=true and validate all types of Linear, VOD content in all apps.
Risks: Medium

@Vinish100 Vinish100 requested a review from a team as a code owner November 13, 2025 09:52
Copilot AI review requested due to automatic review settings November 13, 2025 09:52
Copilot finished reviewing on behalf of Vinish100 November 13, 2025 09:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR integrates the mp4Demux component formally into AAMP to enable demuxing of MP4 fragmented content. The integration adds support for handling both linear and VOD content through a new configuration option eAAMPConfig_UseMp4Demux.

Key Changes:

  • Introduces new Mp4Demux class to parse MP4 boxes and extract media samples with DRM metadata
  • Adds AampMp4Demuxer wrapper class implementing the MediaProcessor interface for AAMP integration
  • Defines new data types (AampCodecInfo, AampMediaSample, AampDrmMetadata) to represent demuxed content
  • Updates GStreamer pipeline integration to handle demuxed samples directly instead of forwarding full segments

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
mp4demux/Mp4Demux.hpp New core MP4 demuxer implementation with box parsing and sample extraction logic
mp4demux/AampMp4Demuxer.h/cpp AAMP integration wrapper that adapts Mp4Demux to MediaProcessor interface
AampDemuxDataTypes.h Common data type definitions for codec info, samples, and DRM metadata
streamabstraction.cpp Updates media processor initialization to conditionally use Mp4Demux based on config
priv_aamp.h/cpp Adds new SendStreamTransfer and SetStreamCaps methods for sample-based transfer
aampgstplayer.h/cpp Updates GStreamer integration to handle MediaSample objects and set stream caps
middleware/InterfacePlayerRDK.h/cpp Defines GStreamer-specific data types and updates SendHelper signature
StreamSink.h Adds abstract SendSample and SetStreamCaps methods to base interface
aampgstplayer.h Updates AAMPGstPlayer SendHelper signature to accept MediaSample
AampStreamSinkInactive.h Adds stub SendSample implementation for inactive streams
mediaprocessor.h Adds getCodecInfo virtual method to MediaProcessor base class
isobmff/isobmffprocessor.h Implements getCodecInfo for ISO BMFF processor
fragmentcollector_mpd.h/cpp Adds GetStreamCodecInfo to retrieve codec information from tracks
test/aampcli/AampcliPlaybackCommand.cpp Updates CLI tool to use new Mp4Demux API with shared_ptr and detailed sample logging
test/qtdemuxAnalyzer/qtdemuxAnalyzer.cpp Improves caps logging by converting full caps to string
test/gstTestHarness/mp4demux.hpp Renames fields (horizresolution→horizontal_resolution, etc.) and fixes spelling
CMakeLists.txt Updates include paths and source list to include mp4demux directory

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{
CodecInfo gstCodecInfo;
gstCodecInfo.codecFormat = (GstStreamOutputFormat)codecInfo.mCodecFormat;
gstCodecInfo.codecData = std::move(codecInfo.mCodecData);
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The std::move(codecInfo.mCodecData) operation may cause issues if the source codecInfo is used again later. Since this is converting between data types, consider using a copy instead: gstCodecInfo.codecData = codecInfo.mCodecData;

Suggested change
gstCodecInfo.codecData = std::move(codecInfo.mCodecData);
gstCodecInfo.codecData = codecInfo.mCodecData;

Copilot uses AI. Check for mistakes.
@rdkcmf-jenkins
Copy link

rdkcmf-jenkins commented Nov 13, 2025

Coverity Issue - Data race condition

Accessing "this->mPauseInjector" without holding lock "InterfacePlayerRDK.mSourceSetupMutex". Elsewhere, "InterfacePlayerRDK.mPauseInjector" is written to with "InterfacePlayerRDK.mSourceSetupMutex" held 2 out of 2 times (2 of these accesses strongly imply that it is necessary).

Medium Impact, CWE-366
MISSING_LOCK

Issue location

This issue was discovered outside the diff for this Pull Request. You can find it at:
middleware/InterfacePlayerRDK.cpp:3067

Copilot AI review requested due to automatic review settings November 13, 2025 12:31
Copilot finished reviewing on behalf of Vinish100 November 13, 2025 12:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 15 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings November 14, 2025 14:56
Copilot finished reviewing on behalf of Vinish100 November 14, 2025 14:58
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings November 17, 2025 08:42
Copilot finished reviewing on behalf of Vinish100 November 17, 2025 08:46
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings November 18, 2025 04:20
Copilot finished reviewing on behalf of Vinish100 November 18, 2025 04:21
Copilot finished reviewing on behalf of Vinish100 November 24, 2025 14:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 39 out of 40 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Vinish100 Vinish100 force-pushed the feature/VPLAY-11143 branch from 092ad05 to ef7a394 Compare December 1, 2025 08:37
Copilot AI review requested due to automatic review settings December 1, 2025 09:08
@Vinish100 Vinish100 force-pushed the feature/VPLAY-11143 branch from ef7a394 to 0067eef Compare December 1, 2025 09:08
Copilot finished reviewing on behalf of Vinish100 December 1, 2025 09:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 52 out of 53 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Vinish100 Vinish100 force-pushed the feature/VPLAY-11143 branch from 0067eef to 4eb8855 Compare December 1, 2025 12:20
* @param[in] frameRate - rate per second
* @return void
*/
void setFrameRateForTM (int frameRate) override { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should spell out "TrickMode" rather than using "TM" abbreviation

/**
* @brief optionally specify new pts offset to apply for subsequently injected TS media segments
*/
void setPtsOffset( double ptsOffset ) override { };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming this for non-invasive pts restamping?

Copilot AI review requested due to automatic review settings December 2, 2025 09:42
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@rdkcmf-jenkins
Copy link

Coverity Issue - Resource leak in object

Allocating memory by calling "new Configs".

High Impact, CWE-401
CTOR_DTOR_LEAK

Issue location

This issue was discovered outside the diff for this Pull Request. You can find it at:
middleware/InterfacePlayerRDK.cpp:77

Copilot AI review requested due to automatic review settings December 4, 2025 12:09
@varshnie varshnie force-pushed the feature/VPLAY-11143 branch from 90f5d84 to 1702846 Compare December 4, 2025 12:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Vinish100 and others added 8 commits December 5, 2025 11:05
Reason for change: Integrate mp4Demux formally into AAMP
Test Procedure: Test with useMp4Demux=true and validate
all types of Linear, VOD content in all apps.
Risks: Low

Signed-off-by: Vinish100 <[email protected]>
Reason for change: Added function headers, split
up Mp4Demux.hpp to a .h and .cpp. Code cleanup
Test Procedure: Play clear and encrypted content using mp4demux
Risks: Low

Signed-off-by: Vinish100 <[email protected]>
Reason for change: Ensure coding guidelines are
strictly followed. Code cleanup. Enforce encrypted
caps are persisted when processing clear samples
in between
Test Procedure: As mentioned in the ticket
Risks: None

Signed-off-by: Vinish100 <[email protected]>
Reason for change: Fix L1s with Mp4Demux
Test Procedure: Test with useMp4Demux=true and validate
all types of Linear, VOD content in all apps.
Risks: Low

Signed-off-by: lashmintha <[email protected]>
Reason for change: Fix review comments. Introduced
error detection and propogation in Mp4Demuxer. Code cleanup
Test Procedure: As mentioned in the ticket
Risks: Low

Signed-off-by: Vinish100 <[email protected]>
Reason for change:pipeline reconfigure is avoided if stream format is unknown
Procedure: As mentioned in the ticket
Risks: Low

Signed-off-by: varshnie <[email protected]>
Reason for change:Removed isobmff format setting hack
Procedure: As mentioned in the ticket
Risks: Low

Signed-off-by: varshnie <[email protected]>
Reason for change: Included possible L1 test for validating Mp4demux functionality,
                     1. Added AampMp4DemuxTests tests which validates SendSegment() in possible scenario's
                     2. Added Mp4BoxParsingtests which validates all the box parsing pssh, saiz, senc, trun, tfhd, moof, tenc, ftyp

Test Procedure: Test with useMp4Demux=true and validate all types of Linear, VOD content in all apps.

Risks: Low

Signed-off-by: lashmintha <[email protected]>
Copilot AI review requested due to automatic review settings December 5, 2025 05:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants